-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Package name validation #1998
Package name validation #1998
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1998 +/- ##
==========================================
- Coverage 46.68% 46.47% -0.21%
==========================================
Files 332 332
Lines 16459 16532 +73
Branches 2195 2231 +36
==========================================
Hits 7684 7684
- Misses 7876 7938 +62
- Partials 899 910 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good in general, but we should change package existence checking code to check for package existence rather than data directory existence. plus some minor issues / questions in inline comments
endpoint: '/package_name_valid', | ||
method: 'POST', | ||
body: { name }, | ||
const list = await requests.bucketListing({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should check for package existence here, not for data existence (either by querying the package index or listing the .quilt/named_packages/$handle
dir)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know anything about this, please, provide more details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see the contents of .quilt/named_packages
dir: it has all the packages registered in the bucket and has the $handle/$pointer_file
structure, so to check if the package exists in the bucket you should list objects under .quilt/named_packages/$handle/
prefix -- if there are any, the package exists, otherwise it does not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added method requests.ensurePackageIsPresent
, where i check for .quilt/named_packages/package_name/latest
.
headObject(".quilt/named_packages/package_name")
(just directory, without package revision) suddenly doesn't work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added method requests.ensurePackageIsPresent, where i check for .quilt/named_packages/package_name/latest.
i think listing revisions is more reliable, bc the latest
tag may be absent for some reason
headObject(".quilt/named_packages/package_name") (just directory, without package revision) suddenly doesn't work
that's expected, bc head request only works for objects, and there's no such object, there's only prefix, so you should use listObjectsV2
for that prefix (you can limit number of keys to 1 to just see if there's anything under that prefix, no matter what)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed requests.ensurePackageIsPresent
. Now it uses request.bucketListing
(which uses listObjectsV2
). I just check if any file under .named_packages/package_name
helperText
(text under Input)Warning messages:
Create package:
Copy package:
Name is pre-filled and validating on start
Update package: